Add disable write stall parameter by githubzilla · Pull Request #153 · eloqdata/store_handler

githubzilla · 2025-11-21T10:04:41Z

Summary by CodeRabbit

New Features
- Added rocksdb_disable_write_stall configuration option, enabling users to control RocksDB write stall behavior for improved database performance and throughput optimization during peak workloads. This new setting is fully configurable via command-line arguments or INI configuration files, and defaults to disabled for maximum backward compatibility with existing system configurations and deployments.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-11-21T10:04:59Z

Walkthrough

A new RocksDB configuration option disable_write_stall is introduced through the configuration system. The flag can be set via command-line or INI config and is propagated from RocksDBConfig through RocksDBDataStoreCommon to be applied as a RocksDB option during database initialization.

Changes

Cohort / File(s)	Summary
Configuration Flag Definition `eloq_data_store_service/rocksdb_config.cpp`	Adds global boolean flag `rocksdb_disable_write_stall` (default: false) and initializes new `disable_write_stall_` member in constructor, reading from CLI flag with INI config fallback.
Configuration Structure `eloq_data_store_service/rocksdb_config.h`	Adds public boolean member variable `disable_write_stall_` to RocksDBConfig struct.
Data Store Configuration `eloq_data_store_service/rocksdb_data_store_common.h`	Adds protected const boolean member `disable_write_stall_` and initializes it from `config.disable_write_stall_` in constructor initializer list.
RocksDB Option Application `eloq_data_store_service/rocksdb_cloud_data_store.cpp`	Adds conditional logic in `OpenCloudDB()` to set `options.disable_write_stall` when `disable_write_stall_` is true, positioned before purger event listener attachment.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Homogeneous changes following existing configuration patterns across files
New option flows consistently through config → data store common → cloud data store
Single conditional application in OpenCloudDB with no complex logic
Areas of focus: Verify the option is applied at the correct point in the initialization sequence and that initialization order doesn't affect subsequent RocksDB option setup

Possibly related PRs

Disable auto compaction at the 1st place to make sure purger blocking… #92: Modifies RocksDB open-time option handling in OpenCloudDB (disabling auto-compactions), similar pattern to this PR's write stall option handling.
Fix slow open rocksdb cloud when db is large #88: Adds other open-time option tweaks to OpenCloudDB, continuing the pattern of configurable RocksDB options during initialization.

Suggested reviewers

liunyl

Poem

🐰 A write stall's been tamed with a flag so divine,
Through config it flows in a steady line,
From CLI to INI, then into the store,
RocksDB options bloom—what could be more?
✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding a new disable_write_stall parameter across the configuration and RocksDB data store components.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch disable_write_stall

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)
599-602: Simplify the conditional logic for boolean option.

The conditional block is unnecessary for a boolean configuration. Other boolean options in this file (e.g., unordered_write, paranoid_checks, atomic_flush) are set unconditionally. The current pattern only makes sense for numeric values where 0 indicates "use RocksDB default."

Apply this diff:
-    if (disable_write_stall_)
-    {
-        options.disable_write_stall = disable_write_stall_;
-    }
+    options.disable_write_stall = disable_write_stall_;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d584a13 and d6effec.

📒 Files selected for processing (4)

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1 hunks)
eloq_data_store_service/rocksdb_config.cpp (2 hunks)
eloq_data_store_service/rocksdb_config.h (1 hunks)
eloq_data_store_service/rocksdb_data_store_common.h (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

eloq_data_store_service/rocksdb_config.cpp (1)

rocksdb_config.cpp (2)

CheckCommandLineFlagIsDefault (298-310)

CheckCommandLineFlagIsDefault (298-298)

🔇 Additional comments (5)

eloq_data_store_service/rocksdb_config.h (1)

69-69: LGTM - clean config member addition.

The new disable_write_stall_ member is properly declared and follows the existing struct conventions.

eloq_data_store_service/rocksdb_config.cpp (2)

101-104: LGTM - proper flag definition.

The flag is correctly defined with a conservative default value (false) and clear documentation.

496-501: LGTM - consistent initialization pattern.

The initialization follows the established pattern: CLI flag takes precedence, falls back to INI config, and uses the flag default as the ultimate fallback.

eloq_data_store_service/rocksdb_data_store_common.h (1)

144-144: LGTM - proper member initialization and declaration.

The disable_write_stall_ member is correctly initialized from the config and declared as a protected const member, following the established pattern for other configuration fields.

Also applies to: 311-311

eloq_data_store_service/rocksdb_cloud_data_store.cpp (1)

599-602: Verification confirms RocksDB API compatibility; alternative flow control exists in codebase.

The disable_write_stall option is confirmed to exist in RocksDB. The code is properly configured with:

Default disabled (safe-by-default via DEFINE_bool(..., false, ...))

Defensive assignment pattern (only applied if explicitly enabled)

Alternative flow control mechanism in place: write_rate_limiter is configured and applied via rocksdb::NewGenericRateLimiter()

Before enabling this setting, verify that:

Your workload requires disabling write stalls (not a performance concern with rate limiting alone)

You have monitoring in place for the risks: unbounded write accumulation, disk exhaustion, and read/compaction performance degradation

The rate limiter configuration is tuned appropriately for your throughput requirements

Add disable write stall

d6effec

githubzilla requested a review from zhangh43 November 21, 2025 10:04

coderabbitai bot reviewed Nov 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add disable write stall parameter#153

Add disable write stall parameter#153
githubzilla wants to merge 1 commit intomainfrom
disable_write_stall

githubzilla commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Nov 21, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

githubzilla commented Nov 21, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

githubzilla commented Nov 21, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 21, 2025 •

edited

Loading